Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maska989/TestStrStrPbrkTok #207

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Maska989/TestStrStrPbrkTok #207

merged 1 commit into from
Sep 13, 2023

Conversation

maska989
Copy link
Contributor

Added

  • strtok(),
  • strtok_r(),
  • strstr(),
  • strpbrk()

function tests.

JIRA: CI-234

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: (list targets here).

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from 8854cd3 to ed22a9c Compare May 22, 2023 07:42
@maska989 maska989 marked this pull request as ready for review May 23, 2023 07:27
@mateusz-bloch mateusz-bloch self-requested a review June 2, 2023 14:51
@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from a58b7f3 to 480ecf8 Compare June 6, 2023 10:45
Copy link
Member

@mateusz-bloch mateusz-bloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just took a quick look, this is not a full review

libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from f2b72a1 to 91ac620 Compare July 6, 2023 12:31
Copy link
Contributor

@damianloew damianloew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's improve these tests a little bit.

libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from c5c801d to 0122b8c Compare July 12, 2023 06:36
@maska989 maska989 requested a review from damianloew July 12, 2023 06:56
Copy link
Member

@mateusz-bloch mateusz-bloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just took a quick look

libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 9 times, most recently from 1bb2699 to 3d3602b Compare July 19, 2023 13:03
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved

memcpy(str2, str1, sizeof(str1));

TEST_ASSERT_EQUAL_PTR(str1, (token = strtok_r(str1, separators, &restState1)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there are 2 variables called restState?

Please assert their values after call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those variables are restrictions for strtok_r to do thread-safe tokenization and to don't mix up tokenizations between two elements similar to themselves. Asserting this value is pointless because it is possible to assert this as not null, but we discard this option using the next tokenization without a mixup.

libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
}


TEST(string_pbrk, basic)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can certainly add more cases to cover this funciton. First example would be strbrk("abc", "") For example:

  • passing bytes in the s2 in various order, so strpbrk("abcde", "dce")
  • passing more bytes (than available in s1) in s2, so sth like: strpbrk("abcde", "cdefgh")
  • big buffers
  • a few same bytes in s2
  • maybe some other cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried different approaches in those cases, I will wait for a response about them.

@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from 30c419e to 281fa94 Compare July 31, 2023 13:23
@maska989 maska989 requested a review from damianloew July 31, 2023 16:03
Comment on lines 131 to 132
TEST_ASSERT_EQUAL_PTR(str, strtok(str, empty));
TEST_ASSERT_EQUAL_PTR(NULL, strtok(NULL, empty));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove those local variables. As I mentioned before empty and "" are bot han empty string, so we can use only "".

Suggested change
TEST_ASSERT_EQUAL_PTR(str, strtok(str, empty));
TEST_ASSERT_EQUAL_PTR(NULL, strtok(NULL, empty));
TEST_ASSERT_EQUAL_PTR(str, strtok("abc", ""));

Copy link
Contributor Author

@maska989 maska989 Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied those changes, but in suggestions is an error because, with the string passed by hand, we can't assert PTR and one line above instead of passing string as "" i was forced to use "\0" (it resolve this problem) because of restrictions in variables passed to strtok(GCC)

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, ok there is restrict in the declaration. Using "\0" seems to be fine in this case.

libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
}


TEST(string_tok_r, restriction)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand, first call puts a NULL pointer in asciiStr[2], so each of next calls will return only "1" as a token, I don't see a purpose of asserting it 255 times.

I suppose you wanted to test mainly the rest there, which in fact is the same for all iterations (I don't see why only two of the last rests have to be equal)

libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
}

free(extAsciiStr);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also create a case, where the separator string contains the same character a few times, same with str, it could contain the same characters multiple times. There is no big case, we can do it at once in such one.

Copy link
Contributor

@damianloew damianloew Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a case, where the separator string contains the same character a few times

Where is such case? I can't see it for strtok() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those cases are checked in multi_call in strtok and strtok_r

@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from f5bb7ab to ca1d9be Compare August 16, 2023 10:29
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
}

free(extAsciiStr);
}
Copy link
Contributor

@damianloew damianloew Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a case, where the separator string contains the same character a few times

Where is such case? I can't see it for strtok() function.

@maska989 maska989 force-pushed the Maska989/TestStrStrBrkTok branch 2 times, most recently from 485105b to 3ee09c5 Compare August 21, 2023 08:45
Copy link
Contributor

@damianloew damianloew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so much left to change :)

libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
libc/string/string_tokpbrkstr.c Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 13, 2023

Unit Test Results

5 721 tests  +224   5 076 ✔️ +175   25m 3s ⏱️ + 1m 44s
   290 suites ±    0      645 💤 +  49 
       1 files   ±    0          0 ±    0 

Results for commit 2578f3d. ± Comparison against base commit aa83bda.

♻️ This comment has been updated with latest results.

@damianloew damianloew merged commit e27b3ef into master Sep 13, 2023
28 checks passed
@damianloew damianloew deleted the Maska989/TestStrStrBrkTok branch September 13, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants